Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(csharp): XML encode contents of <summary> and <example><code> code comments #4785

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Swimburger
Copy link
Contributor

@Swimburger Swimburger commented Oct 1, 2024

When < and > are used inside of summary or code example XML docs, the XML doc is invalid.
This PR HTML encodes the contents before putting it in the XML doc.

This will also XML encode other characters which aren't really necessary, like ", but IDEs continue to show the XML docs correctly.

@Swimburger Swimburger changed the title feat(csharp-sdk): XML encode contents of <summary> and <example><code> code comments feat(csharp): XML encode contents of <summary> and <example><code> code comments Oct 1, 2024
Copy link

github-actions bot commented Oct 1, 2024

@@ -11,7 +11,7 @@ public record User
public required string Id { get; set; }

/// <summary>
/// The user's name. This name is unique to each user. A few examples are included below:
/// The user&#39;s name. This name is unique to each user. A few examples are included below:
Copy link
Contributor

@dcb6 dcb6 Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's looking like lodash's escape might be a little overkill for us -- from my understanding, lodash is going to escape the following chars: & < > " '

But I think that we only need carats to be escaped

Can we roll our own method that only escapes carats?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was on the fence about that. Let's do it ourselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dcb6 The code has been updated with our own escaping.

@Swimburger Swimburger requested a review from dcb6 October 2, 2024 20:42
});
writer.writeLine("/// </code>");
writer.writeLine("/// </example>");
writer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DocXmlWriter is awesome!

and overall i like the ideas that you're bringing here, such as this fluent writer API, but i'd prefer consistency with our existing writer. I'd also like to avoid adding new write methods to our existing writer. So, I have a couple changes to propose:

  1. remove the writeDocXml method from the Writer class and instead just initialize a DocXmlWriter here (and elsewhere)
  2. remove the fluent elements of the DocXmlWriter by simply have the methods return void, and changing the usages accordingly

I ack this is fairly opinionated, but I think we consider it more important to adhere to a consistent style than to introduce new patterns that bring fairly slight improvements

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@dcb6
Copy link
Contributor

dcb6 commented Oct 3, 2024

This part of the description is no longer accurate, correct?

This will also XML encode other characters which aren't really necessary, like ", but IDEs continue to show the XML docs correctly.

@Swimburger
Copy link
Contributor Author

This part of the description is no longer accurate, correct?

This will also XML encode other characters which aren't really necessary, like ", but IDEs continue to show the XML docs correctly.

Correct!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants